Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a LookupScopeStatement node #1742

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

oxisto
Copy link
Member

@oxisto oxisto commented Sep 27, 2024

This PR adds a new node type LookupScopeStatement, which can be used to adjust the lookup scope of symbols that are resolved in the current scope. Most prominent examples are Python's global and nonlocal statements. There is also potentially a global statement in PHP, although we do not support that language.

Potentially, this could be re-used to for concepts that are similar in other languages as well.

Support for python will be added in a later PR. This will only add the node and provides the basic functionality in the lookup.

@oxisto oxisto added the graph-changing This change breaks the current graph structure, i.e. it changes semantic of properties and edges label Sep 27, 2024
This PR adds a new node type `LookupScopeStatement`, which can be used to adjust the lookup scope of symbols that are resolved in the current scope. Most prominent examples are Python's `global` and `nonlocal` statements.

Support for python will be added in a later PR. This will only add the node and provides the basic functionality in the lookup.
Copy link

sonarcloud bot commented Sep 27, 2024

Copy link
Contributor

@maximiliankaul maximiliankaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however, I don't have a sufficiently deep understanding of how the lookup works to click "approve". I assume this PR will be discussed at the CPG hackathon?


// Add it to our scope
for (symbol in symbols) {
node.scope?.predefinedLookupScopes[symbol] = node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this. Don't we want do do something with targetScope? Or add the information to the symbol?
What's the purpose of this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are storing the node in predefinedLookupScopes so that it can later be retrieved by lookupSymbol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graph-changing This change breaks the current graph structure, i.e. it changes semantic of properties and edges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants